These should also be removed as they seem to be properties?

I don't understand what you mean with properties. If you create a thing, these should be definitely provided even though the device could work without it but still I feel they should be editable in case the user wants to overwrite them.

Why would the user need to edit the product name? The binding doesn"t use it for anything, I believe?

The binding does not need it but the to me it is important it is shown.
If the product name is discovered incorrectly the user can overwrite it. I wouldn't say that the binding can be 100% correct in detecting it. It only discovers the SKU but the product name is best guessing.

I can understand @jlaur though. If the binding doesn't actually use the product name, and it neither can be meaningfully discovered, what's the point in showing it in the first place? If it can be discovered properly, it should be a property; if the binding needs it and it can't be discovered properly, it should be a parameter, but if neither applies, I think it shouldn't exist at all.
Don't forget you'll make users think about what to achieve when editing this value. They'll be confused when they hear the answer is 'nothing' ;-)

I removed the two hw/sw versions, I made the macAddress a representation property. I made the device type and hostname readonly which looks nice and good after discovery:

But here is the catch: if someone wants to create device manually, then it does't work

• the macAdress isn't shown at all, so it cannot be entered which is a blocker
• hostname and sku are readonly even here, which is a blocker (at least for the hostname)
• "of course" the properties sw/hw version cannot also not be supplied manually.

MAC address needs to be a parameter, same as hostname. Sorry for being ambiguous above, I thought the relation 'is mandatory' -> 'must be parameter' was clear. For discovery results, those parameters are initialized from the values in the respective withProperty calls in the DiscoveryResult. IOW, the configuration map, which contains the parameter values, is initialized from the property map.

SKU and the versions can probably stay as properties and be updated on thing initialization - similarly to e.g. here

Well, no, I though about that a while: if I can and do discover it (due to a scan being started) then I get all information but when someone adds the thing manually I won't trigger a discovery for that, so in that case the sku and product would stay empty.

It kinda hurts my heart making all of these properties...

And this then is the manual creation:

Is it actually possible in the file definition to provide thing properties?

Maybe this discussion is kind of philosophical in terms of manual creation. The likelihood that someone would actually create it manually is very small. I would even think that manual creating in a thing file is actually pretty hard because you would need to find out the hostname and mac address of that device which is very hard to find out without network knowledge.
To compare that with hues: Do people ever add hue lights manually and if, how would they know all the parameters about that?

Which then would lead to the question, how do people deal with that who don't like the WebUI and want to create their things in files?

Is it actually possible in the file definition to provide thing properties?

I don't think so, but I also don't think there's an actual loss here: properties help with identifying / distinguishing discovery results, but when manually creating things, the user already knows what device he's adding. So while not having the properties in the manual case may not be ideal/perfect, I don't think it's an actual issue.

The likelihood that someone would actually create it manually is very small. I would even think that manual creating in a thing file is actually pretty hard because you would need to find out the hostname and mac address of that device which is very hard to find out without network knowledge. To compare that with hues: Do people ever add hue lights manually and if, how would they know all the parameters about that?

Which then would lead to the question, how do people deal with that who don't like the WebUI and want to create their things in files?

I create all my Things manually. Sometimes I need to use discovery first and create a managed Thing. I can then copy the configuration parameters into my manually created Thing in a .things file and delete the managed Thing again.

You don't need to declare these:

However, you should probably in this place declare:

<representation-property>macAddress</representation-property>

As you see in the above discussion the macAdress needs to stay a parameter as it would otherwise not be possible to provide on manual creation.

This is still pending.

I'm sorry, but this is still not safe, since write operations are not synchronized by the same lock. I have now had a closer look trying to follow the flow of events.

Please correct me if I'm reading this flow wrongly.

• Handler A initializes.
• Handler A adds itself to the static THING_HANDLERS.
• Handler A schedules a future which will create a multicast socket and receive one packet from it.
• This future sets the public static refreshJobRunning in GoveeHandler to true.
• It will then get a Map with all thing handlers from static method in GoveeHandler.
• If the received multicast packet matches the IP address of any handler in the received Map, it will then call methods directly on that thing handler instance to update state and status.

If this is correct, I think there are some design flaws to address.

If handler A and handler B initializes at the same time:

• Both can put into the same HashMap at the same time. This will not work.
• triggerStatusJob can be checked at the same time, in which case two futures will be scheduled, and you'll only keep a reference to the last one of them. Therefore there'll be a wild-running future after that.
• When the multicast future gets the thing handlers, this might happen while handler C is adding itself to that Map. This can cause an exception to be thrown.

In addition to this:

• There are issues when discovery is running at the same time, which you have tried to work around. It looks like this could have some concurrency issues also.
• The multicast socket is recreated every second and receives only one packet. This seems very unusual. I think one would normally keep a loop running to receive all packets as soon as they come in, and not miss any. See for example the implementation in the miele binding, or perhaps just start out with grep -R "new MulticastSocket".

The only solution I can think of to address all these issues at the same time would be to add a bridge handler and let this handler be responsible for having the multicast listener running and notify discovery service as well as all handlers. This is done for example by the miele binding.

I realize there is no physical or logical bridge really, so it would be kind of a "pseudo" bridge.

@openhab/add-ons-maintainers (or @maniac103 🙂) - perhaps someone else has better ideas, for example how to fix this design without introducing a bridge?

This is all still full of static use, which imho isn't much different to having the static map in the handlers before.
My suggestion in https://github.com/openhab/openhab-addons/pull/15696/files/dcc8e884219a5828b8db7971a36b2c9877701b5e#r1421568312 was to have a dedicated instance/service that does the handling. Is there any reason why this cannot be done?

This essentially is a singleton doing the communication. If you prefer it can be converted to an actual singleton instance, but I don't see much difference either way, because the singleton instance holding the data will also be static.
Is this what you have in mind or what kind of service are you thinking of exactly?

Singletons are an anti-pattern in the OSGi world. Instead, it should be a service that is instantiated once and used where needed. This prevents lifecycle issues, e.g. when a bundle is stopped, updated and started - singletons are not properly discarded in such cases, while services are.

Stopping bundles should dispose the handlers though, thus making them unsubscribe from and eventually stop the thread?
Got any reference example for such a shared service?

I'm just preparing a (untested) PR that shows what I mean.

Yes, it helped very much indeed!

Were you able to test if the binding still works this way?

@kaikreuzer Actually it is crashing during startup.

``
2023-12-17 13:03:38.739 [ERROR] [.govee.internal.CommunicationManager] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.CommunicationManager(339)] : Constructor with 0 arguments not found. Component will fail.

...

2023-12-17 13:03:38.743 [ERROR] [.govee.internal.CommunicationManager] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.CommunicationManager(339)] : Error during instantiation of the implementation object: Constructor not found.

2023-12-17 13:03:38.748 [WARN ] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Could not get service from ref {org.openhab.binding.govee.internal.CommunicationManager}={service.id=527, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.CommunicationManager, component.id=339}
2023-12-17 13:03:38.749 [ERROR] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Error during instantiation of the implementation object: Unable to get service for reference $002
2023-12-17 13:03:38.750 [DEBUG] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Failed creating the component instance; see log for reason
2023-12-17 13:03:38.751 [WARN ] [nternal.DiscoveryServiceRegistryImpl] - bundle org.openhab.core.config.discovery:4.1.0.202311121345 (167)[org.openhab.core.config.discovery.internal.DiscoveryServiceRegistryImpl(116)] : Could not get service from ref {org.openhab.core.config.discovery.DiscoveryService}={service.id=528, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.GoveeDiscoveryService, component.id=340}
2023-12-17 13:03:38.752 [WARN ] [nternal.DiscoveryServiceRegistryImpl] - bundle org.openhab.core.config.discovery:4.1.0.202311121345 (167)[org.openhab.core.config.discovery.internal.DiscoveryServiceRegistryImpl(116)] : DependencyManager : invokeBindMethod : Service not available from service registry for ServiceReference {org.openhab.core.config.discovery.DiscoveryService}={service.id=528, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.GoveeDiscoveryService, component.id=340} for reference DiscoveryService
``

Maybe stefan-hoehn#2 helps?

Yes, it does. I proudly ;-) discovered the issue myself at the same time.
Testing looks awesome. All devices are discovered and I am able to control them as well.